Add search for a trusted host in ProxyHeadersMiddleware #591

b0g3r · 2020-03-17T09:20:05Z

Fixes #590

If we have * in trusted proxies we should return the first host in the X-Forwarded-For as the client address.

In other cases, we should iterate over X-Forwarded-For hosts backward and return the first host which isn't included in trusted proxies

b0g3r · 2020-03-22T07:40:59Z

I found a discussion about this feature: 723d3dc#r36079732

So, I'll try to find examples from other servers with this feature and make comparison

b0g3r · 2020-03-22T08:37:17Z

uvicorn/middleware/proxy_headers.py

        else:
-            self.trusted_hosts = trusted_hosts
+            self.trusted_hosts = set(trusted_hosts)


For speedup str not in collection operations

iamlordaubrey

From my understanding, the idea here is to return the actual client ip address. This as it is returns the last ip address (which could be a proxy address)

b0g3r · 2020-04-02T16:50:30Z

uvicorn/middleware/proxy_headers.py

+        if self.always_trust:
+            return x_forwarded_for_hosts[0]
+
+        for host in reversed(x_forwarded_for_hosts):


It's just a security feature: we should return only the last non-trusted host.

Trivial case:

Proxy-server A

Proxy-server B

Client C

We trust both of proxies (trusted_hosts = A, B) or trust every proxy (trusted_hosts = *)

So in this scenario, a header will be X-Forwarded-For: C, B, A and we can securely extract C as a trusted client address.

Unfortunately, IRL we have a situation when we have a chain of proxies, and we can trust only a few of them, e.g:

Proxy-server A

Proxy-server B

Client C

We trust only A (trusted_hosts = A)

In this case, non-trusted proxy-server B can replace the real client address, so we should return the first non-trusted host and it will be B. Because the chain is actually reverted, we should check it also with reversed.

tl;dr: In a simple case, we can trust all of the proxies with trusted_hosts = * and it will return real client address (the first in X-Forwarded-For)

b0g3r · 2020-04-02T16:50:30Z

uvicorn/middleware/proxy_headers.py

+        if self.always_trust:
+            return x_forwarded_for_hosts[0]
+
+        for host in reversed(x_forwarded_for_hosts):


It's just a security feature: we should return only the last non-trusted host.

Trivial case:

Proxy-server A

Proxy-server B

Client C

We trust both of proxies (trusted_hosts = A, B) or trust every proxy (trusted_hosts = *)

So in this scenario, a header will be X-Forwarded-For: C, B, A and we can securely extract C as a trusted client address.

Unfortunately, IRL we have a situation when we have a chain of proxies, and we can trust only a few of them, e.g:

Proxy-server A

Proxy-server B

Client C

We trust only A (trusted_hosts = A)

In this case, non-trusted proxy-server B can replace the real client address, so we should return the first non-trusted host and it will be B. Because the chain is actually reverted, we should check it also with reversed.

tl;dr: In a simple case, we can trust all of the proxies with trusted_hosts = * and it will return real client address (the first in X-Forwarded-For)

iamlordaubrey · 2020-04-02T17:56:00Z

From my understanding, the idea here is to return the actual client ip address. This as it is returns the last ip address (which could be a proxy address)

Ok, I think I get it now. Thanks

rlittlefield · 2020-05-28T05:41:25Z

This change is working well for me. The old way of specifying N proxies worked better for my use case because google cloud load balancers + kubernetes services always adds two hosts to the header, and I don't get to know what those are in advance. I can get this to work with your changes. Thank you!

iamlordaubrey · 2020-06-05T15:31:20Z

@tomchristie please can someone take a look at this PR? Sorry for the inconvenience 🙏

ghost · 2020-06-10T08:30:28Z

Is there any chance to release this any time soon?
Slow API (https://pypi.org/project/slowapi/) can not work without this change.

tomchristie · 2020-06-10T10:01:45Z

uvicorn/middleware/proxy_headers.py

+
+        for host in reversed(x_forwarded_for_hosts):
+            if host not in self.trusted_hosts:
+                return host


Do we need a fall through case so there's no way we can end up returning None here?

I fail to see how it would be possible

It would happen if all the IPs in the chain were explicitly listed in trusted_hosts - but this would only occur if trusted_hosts were incorrectly set and/or the first reverse proxy in the chain failed to set x-forwarded-for to the client's IP (not our fault).

In this case, is there any other justifiable return value than None? I can't think of one

euri10

lgtm, this would be a great addition, is there something we can do @tomchristie to alleviate your concerns on the above point ?

dp-rufus · 2021-03-01T18:39:00Z

@euri10 I'm not sure a fall-through case is necessary above - None might be the most sensible return value under the (unlikely) circumstances that would trigger this behaviour.

@tomchristie could you take another look at this PR please? I've just been bitten by this problem in the wild and it would be good to get this merged. Thanks

euri10 · 2021-03-02T14:34:40Z

updated it in https://github.com/encode/uvicorn/compare/master...euri10:b0g3r-uvicorn-590?expand=1 to solve conflicts because of the "new" test suite (original PR was done previous to the test suite being "revamped")

…ed-for` Also rewrite a few tests for increasing coverage for edge cases

b0g3r · 2021-03-13T13:13:31Z

@euri10 Thank you for your comment!

I rebased this PR onto master and fix related issues (127.0.0.1 instead of testclient and others.) The last piece to merge is code review, right?

euri10

just correct this small typo in the comment and we gtg I think, this is a great addition thanks @b0g3r

tests/middleware/test_proxy_headers.py

Co-authored-by: euri10 <[email protected]>

zffocussss · 2022-04-13T02:07:23Z

uvicorn/middleware/proxy_headers.py

+                    x_forwarded_for_hosts = [
+                        item.strip() for item in x_forwarded_for.split(",")
+                    ]
+                    host = self.get_trusted_client_host(x_forwarded_for_hosts)


what if the x-forwarded-for format is IP1:PORT1,IP2:PORT2,which is common in the L7 load balance? the port here is always set zero which is not reasonable.

* Fix ProxyHeadersMiddleware, now it grabs the first IP from `x-forwarded-for` Also rewrite a few tests for increasing coverage for edge cases * Add searching algorithm for the trusted client host in x-forwarded-for * Fix issues after rebasing * Fix typo in comment Co-authored-by: euri10 <[email protected]> Co-authored-by: euri10 <[email protected]>

b0g3r force-pushed the uvicorn-590 branch from abbeb5d to a36acdf Compare March 17, 2020 09:28

b0g3r commented Mar 22, 2020

View reviewed changes

b0g3r referenced this pull request Mar 22, 2020

Add support for --forwarded-allow-ips

723d3dc

b0g3r force-pushed the uvicorn-590 branch from f418bfe to 27e7cca Compare March 22, 2020 08:40

b0g3r changed the title ~~Fix ProxyHeadersMiddleware, now it grabs the first IP from x-forwarded-for~~ Add search for a trusted host in ProxyHeadersMiddleware Mar 22, 2020

iamlordaubrey reviewed Apr 2, 2020

View reviewed changes

b0g3r commented Apr 2, 2020

View reviewed changes

ghost mentioned this pull request May 26, 2020

Limit rate issue laurentS/slowapi#2

Closed

ghost approved these changes Jun 10, 2020

View reviewed changes

tomchristie reviewed Jun 10, 2020

View reviewed changes

euri10 reviewed Sep 29, 2020

View reviewed changes

euri10 requested a review from tomchristie October 23, 2020 08:02

euri10 mentioned this pull request Nov 12, 2020

Fix issues surrounding X-Forwarded-For header in ProxyHeadersMIddleware #701

Merged

b0g3r added 2 commits March 13, 2021 15:35

Fix ProxyHeadersMiddleware, now it grabs the first IP from `x-forward…

603f5fb

…ed-for` Also rewrite a few tests for increasing coverage for edge cases

Add searching algorithm for the trusted client host in x-forwarded-for

b450172

b0g3r force-pushed the uvicorn-590 branch from 27e7cca to b450172 Compare March 13, 2021 13:08

Fix issues after rebasing

ce7a211

euri10 approved these changes Mar 13, 2021

View reviewed changes

tests/middleware/test_proxy_headers.py Outdated Show resolved Hide resolved

Fix typo in comment

05f43a9

Co-authored-by: euri10 <[email protected]>

euri10 merged commit ab2d090 into encode:master Mar 13, 2021

jchristgit mentioned this pull request Jun 2, 2021

Using ProxyHeadersMiddleware with client on trusted proxy results in invalid client information #1068

Closed

2 tasks

Kludex mentioned this pull request Feb 1, 2022

Remove return statement that is never reached on get_trusted_client_host() method #1349

Closed

zffocussss reviewed Apr 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add search for a trusted host in ProxyHeadersMiddleware #591

Add search for a trusted host in ProxyHeadersMiddleware #591

b0g3r commented Mar 17, 2020 •

edited

Loading

b0g3r commented Mar 22, 2020

b0g3r Mar 22, 2020

iamlordaubrey left a comment

b0g3r Apr 2, 2020 •

edited

Loading

b0g3r Apr 2, 2020 •

edited

Loading

iamlordaubrey commented Apr 2, 2020 •

edited

Loading

rlittlefield commented May 28, 2020

iamlordaubrey commented Jun 5, 2020

ghost commented Jun 10, 2020

tomchristie Jun 10, 2020

euri10 Sep 29, 2020

dp-rufus Mar 1, 2021

euri10 left a comment

dp-rufus commented Mar 1, 2021

euri10 commented Mar 2, 2021

b0g3r commented Mar 13, 2021

euri10 left a comment

zffocussss Apr 13, 2022

Add search for a trusted host in ProxyHeadersMiddleware #591

Add search for a trusted host in ProxyHeadersMiddleware #591

Conversation

b0g3r commented Mar 17, 2020 • edited Loading

b0g3r commented Mar 22, 2020

b0g3r Mar 22, 2020

Choose a reason for hiding this comment

iamlordaubrey left a comment

Choose a reason for hiding this comment

b0g3r Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

b0g3r Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

iamlordaubrey commented Apr 2, 2020 • edited Loading

rlittlefield commented May 28, 2020

iamlordaubrey commented Jun 5, 2020

ghost commented Jun 10, 2020

tomchristie Jun 10, 2020

Choose a reason for hiding this comment

euri10 Sep 29, 2020

Choose a reason for hiding this comment

dp-rufus Mar 1, 2021

Choose a reason for hiding this comment

euri10 left a comment

Choose a reason for hiding this comment

dp-rufus commented Mar 1, 2021

euri10 commented Mar 2, 2021

b0g3r commented Mar 13, 2021

euri10 left a comment

Choose a reason for hiding this comment

zffocussss Apr 13, 2022

Choose a reason for hiding this comment

b0g3r commented Mar 17, 2020 •

edited

Loading

b0g3r Apr 2, 2020 •

edited

Loading

b0g3r Apr 2, 2020 •

edited

Loading

iamlordaubrey commented Apr 2, 2020 •

edited

Loading